Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(#3037): add API node.buffer.delete, node.buffer.wipe #3040

Merged
merged 16 commits into from
Jan 25, 2025

Conversation

GCrispino
Copy link
Collaborator

@GCrispino GCrispino commented Dec 30, 2024

This PR adds two API methods for respectively deleting and wiping the buffer for the current file under the tree cursor:

  • Api.node.buffer.delete; and
  • Api.node.buffer.wipe,

fixes #3037.

They both use the same underlying function defined in the new file added at lua/actions/node/utils.lua.
This function checks if there's an opened and loaded buffer for the file under the tree cursor.
If there isn't, it notifies an error message.

Otherwise, it deletes/wipes the buffer if it's not modified (unless opts.force is true)

I'm setting this as WIP because I haven't run the make commands yet.
EDIT: done

@GCrispino GCrispino changed the title WIP: feat(#2826): add node API methods for deleting/wiping buffers WIP: feat(#3037): add node API methods for deleting/wiping buffers Dec 30, 2024
@GCrispino GCrispino changed the title WIP: feat(#3037): add node API methods for deleting/wiping buffers feat(#3037): add node API methods for deleting/wiping buffers Dec 31, 2024
Copy link
Member

@alex-courtis alex-courtis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This works nicely, I look forward to using it.

  • Revert ApiTreeToggleOpts change
  • Change warning level of no-loaded-buffer
  • Remove unused options delete_buffer and wipe_buffer
  • Refactor action methods, message to follow
  • Add help at 6.3 API NODE nvim-tree-api.node

@@ -131,7 +132,7 @@ end
Api.tree.open = wrap(actions.tree.open.fn)
Api.tree.focus = Api.tree.open

---@class ApiTreeToggleOpts
---@class ApiTreeToggleOptsApiTreeTo
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the reason for this change? lua/nvim-tree/actions/tree/toggle.lua is still using this class...

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No idea 😅 , this was probably done accidentally. Just reverted it 👍🏼

@@ -286,6 +287,16 @@ Api.node.navigate.diagnostics.prev_recursive = wrap_node(actions.moves.item.fn({
Api.node.navigate.opened.next = wrap_node(actions.moves.item.fn({ where = "next", what = "opened" }))
Api.node.navigate.opened.prev = wrap_node(actions.moves.item.fn({ where = "prev", what = "opened" }))

---@class ApiNodeDeleteWipeBufferOpts
---@field force boolean|nil default false
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you.

@@ -470,6 +470,8 @@ local DEFAULT_OPTS = { -- BEGIN_DEFAULT_OPTS
remove_file = {
close_window = true,
},
delete_buffer = {},
wipe_buffer = {},
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are these options used?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, I was unsure if it was a convention to have these even if there are no underlying options, so I added these in even though they are empty.

I can remove them 👍🏼

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested permutations and combination with :ls! reporting as expected.

  vim.keymap.set("n", "BW", function()
    api.node.buffer.wipe(api.tree.get_node_under_cursor(), { force = false })
  end, opts("Wipe"))

  vim.keymap.set("n", "BD", function()
    api.node.buffer.wipe(api.tree.get_node_under_cursor(), { force = true })
  end, opts("Delete"))

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great! Thank you 👍🏼

-- check if buffer for file at cursor exists and if it is loaded
local bufnr_at_filename = vim.fn.bufnr(filename)
if bufnr_at_filename == -1 or vim.fn.getbufinfo(bufnr_at_filename)[1].loaded == 0 then
notify.error("No loaded buffer coincides with " .. notify_node)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we please change this to INFO? Users may wish to ignore this message.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, sure! Should I also change the error statement at line 37 (the one that says "Buffer for file " .. notify_node .. " is modified"), or just this one?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think just this one. Deleting a modified buffer truly is an error.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, done!

@alex-courtis
Copy link
Member

alex-courtis commented Jan 13, 2025

I like the modularity, it's really easy to read.

Having 3 new files is not necessary, delete and wipe can be handled in one place:
Create a single file buffer.lua: containing the utils function and fn( methods renamed to wipe and delete.
We don't really need the fn() pattern any more.

These actions do act on a node, and belong in actions/node. Rather than the absolute_path being passed in, the node itself should be passed in:

  1. Prevents error when the user passes a node without that field.
  2. Allows testing of the node's type as a FileNode. We don't want to try and delete directories or the tree node itself. That can silently exit.

@GCrispino
Copy link
Collaborator Author

GCrispino commented Jan 15, 2025

I like the modularity, it's really easy to read.

Having 3 new files is not necessary, delete and wipe can be handled in one place: Create a single file buffer.lua: containing the utils function and fn( methods renamed to wipe and delete. We don't really need the fn() pattern any more.

These actions do act on a node, and belong in actions/node. Rather than the absolute_path being passed in, the node itself should be passed in:

  1. Prevents error when the user passes a node without that field.
  2. Allows testing of the node's type as a FileNode. We don't want to try and delete directories or the tree node itself. That can silently exit.

@alex-courtis

Ok, when I get some free time I'll work on these changes. Thanks for the review :)

@GCrispino
Copy link
Collaborator Author

@alex-courtis I've pushed some commits, and I believe I have addressed all of your issues :)

Kindly check when you can 🙏🏼 , thanks!

@alex-courtis
Copy link
Member

Looks like we have some newly deprecated nvim methods : https://github.com/nvim-tree/nvim-tree.lua/actions/runs/12939265657/job/36091177064?pr=3040

I'll resolve them on master and merge.

Copy link
Member

@alex-courtis alex-courtis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the interests of speed, I pushed fixes for the review comments.

Please let me know if you'd prefer me not to push.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like this file can go - everything is in buffer.lua

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh yeah, we can remove this, I forgot to do so, sorry!

end

function M.setup(_)
end
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can remove this NOP; we're moving away from this setup pattern anyway.

---@param opts ApiNodeDeleteWipeBufferOpts|nil
---@return nil
function M.delete(node, opts)
M.delete_buffer("delete", node.absolute_path, opts)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This handles nil or partial node gracefully, however not a malformed node like { aaaabsolute_path = "foo" }

That's OK - garbage in, garbage out.

@GCrispino
Copy link
Collaborator Author

Looks like we have some newly deprecated nvim methods : https://github.com/nvim-tree/nvim-tree.lua/actions/runs/12939265657/job/36091177064?pr=3040

I'll resolve them on master and merge.

@alex-courtis all good for me 🫡 ! Thanks for jumping in and resolving these 🙏🏼

Copy link
Member

@alex-courtis alex-courtis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Many thanks for the fantastic addition

@alex-courtis alex-courtis changed the title feat(#3037): add node API methods for deleting/wiping buffers feat(#3037): add API node.buffer.delete, node.buffer.wipe Jan 25, 2025
@alex-courtis alex-courtis merged commit fee1da8 into nvim-tree:master Jan 25, 2025
6 checks passed
@GCrispino
Copy link
Collaborator Author

Many thanks for the fantastic addition

Thanks for that, and for assisting here! Would love to contribute again if there are any good opportunities for newcomers

@alex-courtis
Copy link
Member

Many thanks for the fantastic addition

Thanks for that, and for assisting here! Would love to contribute again if there are any good opportunities for newcomers

It's a pleasure to work with you and it'd be great to have another team member on board.

There's a PR Please label on many features / bugs - issues that we just don't have the bandwidth for.

Alternatively, there's a larger project for Multiple Instances under way. This task on the Project Board would be a good start - it's got a clear goal and definition of done.

@alex-courtis
Copy link
Member

Added you as a project member so that you can push branches, review PRs, take tasks etc.

@GCrispino
Copy link
Collaborator Author

It's a pleasure to work with you and it'd be great to have another team member on board.

Thank you! Likewise, it was a pleasure to contribute :)

There's a PR Please label on many features / bugs - issues that we just don't have the bandwidth for.

Alternatively, there's a larger project for Multiple Instances under way. This task on the Project Board would be a good start - it's got a clear goal and definition of done.

Oh, nice! I think I'll pick up a bug so I get a bit more familiar with the project and Lua, as I'm not too familiar with the language and writing good code on it.
The Multiple Instances project sounds exciting!

@GCrispino
Copy link
Collaborator Author

Added you as a project member so that you can push branches, review PRs, take tasks etc.

Oh and of course, thanks for this! This is much appreciated :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add keymap for closing buffer of file under cursor
2 participants